feat(codex): add encrypted native main profiles - #863
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughNative Codex main profiles now support encrypted storage, staged login, switching, recovery, startup fencing, CLI and management APIs, quota identity fencing, and extensive validation. CI and release workflows add high-severity dependency audits and cross-platform keyring smoke tests. ChangesNative Codex profile management
Audit and keyring safeguards
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant ManagementAPI
participant NativeProfileManager
participant CodexAuth
participant StartupGate
CLI->>ManagementAPI: register, switch, or recover profile
ManagementAPI->>NativeProfileManager: validate and execute operation
NativeProfileManager->>CodexAuth: replace and verify native auth envelope
NativeProfileManager->>StartupGate: publish transition or complete recovery
StartupGate->>CodexAuth: allow native main traffic after recovery
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Maintainer security review on head Must change before merge (priority order):
Items 1-3 are the true blockers. If you are short on cycles, say so — a maintainer will take over on top of your branch. This is genuinely close to the capability we want to ship. |
9a2c1fa to
fc1f6ab
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc1f6ab7a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 218-236: The non-Linux branch of the “OS keyring
create/read/delete smoke” workflow runs directly against persistent self-hosted
keyring state. Update this step to isolate the self-hosted Windows leg from the
real Credential Manager, or add reliable pre- and post-job cleanup that removes
all lingering opencodex.keyring-smoke.* entries, including when the smoke test
is interrupted.
In `@docs-site/src/content/docs/reference/cli/providers-accounts.md`:
- Around line 213-232: Synchronize the `ocx account main` documentation across
the English, Japanese, Korean, Russian, and Simplified Chinese
`providers-accounts.md` pages. Update the English command block to use `<label>`
for profile creation, `<profile-id-or-label>` for `switch`, retain `switch
--yes`, and document every supported `--json` flag according to
`src/cli/account-main.ts` and its `resolveTarget` behavior; add the
corresponding section to the three non-English locale pages.
In `@scripts/keyring-smoke.ts`:
- Around line 34-51: The nested cleanup in runKeyringSmoke must not mask a
pending readback-mismatch error: in scripts/keyring-smoke.ts lines 34-51, log
deleteCredential failure instead of re-throwing it. Add a regression case in
tests/keyring-smoke.test.ts lines 44-56 where getSecret mismatches and
deleteCredential fails, asserting runKeyringSmoke rejects with the original
“readback did not match” message.
In `@src/codex/account-lifecycle.ts`:
- Around line 43-60: Extract the repeated main-account cleanup sequence from
reconcileMainCodexAccountRuntimeState and
applyConfirmedMainCodexAccountTransition into a shared helper. Have that helper
call purgeCodexAccountRuntimeState, setMainAccountPlan(null), and
invalidateCodexWebSocketsForAccount for MAIN_CODEX_ACCOUNT_ID, then replace both
inline sequences with the helper while preserving transition behavior.
In `@src/codex/native-profile-api.ts`:
- Around line 101-109: Update the generic fallback branch in the native-profile
error handler to return a distinct internal-failure code such as
"INTERNAL_ERROR" instead of "RECOVERY_REQUIRED". Preserve "RECOVERY_REQUIRED"
exclusively for the NativeProfileError path emitted by assertNoPendingRecovery.
In `@src/codex/native-profile-manager.ts`:
- Around line 494-507: Refactor the operation around the existing native profile
import body and cleanup so deleteStageById never throws from finally. Always
perform buffer zeroization and staging cleanup, capture cleanup failure,
preserve and rethrow any original operation error, and only raise
STAGING_CLEANUP_REQUIRED after a successful writeVault result. Update that error
message to state the profile was imported before cleanup failed, then return the
committed profile normally when cleanup succeeds.
- Around line 310-344: Update doctor() to catch failures from sweepStaleStages()
and readNativeProfileVault(), recording degraded classifications in new
vaultStatus and stagingSweep fields while preserving the existing diagnostic
report. Ensure corrupt vaults still return profileCount and activeProfileId
safely, and staging cleanup failures do not abort the report; keep
credential-store, auth, and recoveryPending diagnostics available.
In `@src/codex/native-profile-processes.ts`:
- Line 10: Validate the SystemRoot-derived executable path before the PowerShell
invocation in the native profile process flow, using the existing C:\Windows
fallback whenever the environment value is absent or implausible. Ensure
execFileSync uses only the validated Windows system path and does not execute a
location selected from an untrusted writable directory.
- Around line 17-43: Set an explicit sufficiently large maxBuffer on both
execFileSync calls in powershellProcessCount and unixProcessCount, including the
ps output path with full arguments. Preserve the existing counting and
validation behavior while preventing expected process-list output from raising
ENOBUFS and being downgraded by probeNativeCodexProcesses to unknown.
In `@src/codex/native-profile-recovery.ts`:
- Around line 78-91: Update decideNativeProfileRecovery to consume
observation.digest and set an externallyRefreshed signal when the target has a
changed digest during the commit-recovery path, while preserving the existing
commit-target action. Add that signal to NativeProfileRecoveryDecision and
propagate it through recoverLocked’s returned object so CLI and management API
callers can report the external refresh; otherwise remove the unused digest
field if warning behavior is intentionally deferred.
In `@src/codex/native-profile-store.ts`:
- Around line 358-375: Separate malformed or unreadable journals from valid
pending journals in readNativeProfileJournal, while preserving RECOVERY_REQUIRED
for normal fail-closed credential operations. Update recover/recoverLocked so an
explicit confirmed rollback catches the malformed-journal case, renames
context.journalPath to a unique quarantine filename using the transaction ID or
timestamp, leaves auth.json and the vault unchanged, and returns the quarantine
path. Keep register, switching, and non-confirmed recovery blocked.
- Around line 381-387: Update validateNativeProfileLabel and parseVaultObject to
share a predicate that rejects bidi override/isolate characters and zero-width
characters in addition to the existing control-character checks. Define the
predicate once near the profile-label validation logic, reuse it in both
validation paths, and preserve trimming, length limits, and the existing
INVALID_REQUEST error behavior.
In `@src/codex/native-profile-types.ts`:
- Line 68: Remove the duplicate NativeProfileJournalPhase declaration from
native-profile-recovery.ts and import/re-export the canonical type from
native-profile-types.ts. Keep consumers such as decideNativeProfileRecovery
using the shared type so native-profile-store.ts and recovery logic remain
synchronized.
In `@tests/helpers/native-profile-lock-child.ts`:
- Around line 19-23: Change the simulated crash branch in onLockAcquired to exit
with the distinct non-zero code used by native-profile-switch-child.ts, then
update the corresponding assertion in native-profile-manager.test.ts to expect
that code so the test verifies the crash path executed.
In `@tests/helpers/native-profile-startup-child.ts`:
- Around line 71-77: Update the promise chain around
waitForNativeMainStartupGate() to attach a rejection handler that writes the
failure details into settledPath, including errors thrown by
nativeMainStartupGateSnapshot(), isMainAccountTokenLive(), or loadConfig() in
the success callback. Preserve the existing success payload and ensure every
failure path writes a JSON record so parent tests surface the cause instead of
timing out.
In `@tests/native-profile-api.test.ts`:
- Around line 51-79: Update the test “stale HTTP/Responses-WebSocket work
settles before switch and new turns stay fenced” to release both admission
leases with try/finally cleanup: always release oldTurn after the request flow,
and always release after when it is acquired. Preserve the existing assertions
and ordering checks while ensuring cleanup runs if the drain returns early or
any assertion throws.
- Line 114: Update the username-leak assertion in the native-profile API test to
read the current OS account name from a cross-platform source that resolves on
Windows, Linux, and macOS, rather than relying only on process.env.USERNAME.
Keep the assertion focused on ensuring the redacted management error payload
does not contain that resolved identity.
In `@tests/native-profile-crash-boundaries.test.ts`:
- Around line 17-21: Restore captured environment variables through a shared
helper that deletes the key when the captured value is undefined, otherwise
assigns the saved value. Apply this in
tests/native-profile-crash-boundaries.test.ts at lines 17-21 and 70-71, and in
tests/native-profile-startup.test.ts at lines 34-38 and 171-172, covering the
afterEach hooks and mid-fixture restores.
- Around line 196-216: Wrap the concurrent-switch test body in a finally block
that always writes firstRelease and awaits both first.exited and second.exited,
including when waitFor or an assertion fails. Mirror the cleanup pattern used by
the nearby test and the existing native-profile-manager test, while preserving
the current success assertions.
In `@tests/native-profile-manager.test.ts`:
- Around line 85-87: Update the journal-phase checks in the test’s injected
failure logic at both matching locations to parse content with JSON.parse and
compare its phase property to "auth-replaced". Replace formatting-dependent
substring matching while preserving the existing path condition and failure
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7593e26e-ba06-4588-acb8-b66bf30f340a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/workflows/ci.yml.github/workflows/release.ymldevlog/_plan/260801_native_main_profiles/000_design.mddevlog/_plan/260801_native_main_profiles/001_validation.mddevlog/_plan/260801_native_main_profiles/002_implementation.mddocs-site/src/content/docs/reference/cli/providers-accounts.mdpackage.jsonscripts/keyring-smoke.tsscripts/release.tssrc/cli/account-api.tssrc/cli/account-main.tssrc/cli/account.tssrc/cli/help.tssrc/codex/account-lifecycle.tssrc/codex/account-usability.tssrc/codex/auth-api.tssrc/codex/auth-context.tssrc/codex/native-profile-api.tssrc/codex/native-profile-manager.tssrc/codex/native-profile-processes.tssrc/codex/native-profile-recovery.tssrc/codex/native-profile-startup.tssrc/codex/native-profile-store.tssrc/codex/native-profile-types.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-api.tssrc/server/management/context.tstests/cli-native-profile.test.tstests/codex-websocket-registry.test.tstests/helpers/native-profile-lock-child.tstests/helpers/native-profile-startup-child.tstests/helpers/native-profile-switch-child.tstests/keyring-smoke.test.tstests/native-profile-api.test.tstests/native-profile-crash-boundaries.test.tstests/native-profile-manager.test.tstests/native-profile-recovery.test.tstests/native-profile-route-security.test.tstests/native-profile-startup.test.ts
fc1f6ab to
8790758
Compare
Draft checkpoint updatePushed This checkpoint includes the implementation for all 27 existing review threads plus the follow-up hardening completed during integration: canonical Local validation on Bun
The PR intentionally remains Draft. I will wait for hosted CI, then perform thread-by-thread readback before resolving comments or marking it Ready for review. |
|
Review-thread readback against the public head 8790758 is complete. I resolved 26 of 27 threads whose fixes and regression coverage are present in this revision. I intentionally left only the hosted keyring runner-isolation thread open until the Ubuntu, Windows, and macOS keyring jobs actually run on this head. The PR remains Draft while that hosted evidence and the latest dev refresh are pending. |
8790758 to
9273337
Compare
|
Draft checkpoint updated onto the latest
The PR remains Draft. The hosted keyring-smoke review thread stays open until the disposable Ubuntu, Windows, and macOS jobs have actually run. |
|
@Wibias The latest Draft head is ready for a Ready-state decision:
Would you recommend marking this PR Ready now so review and hosted workflow approval can proceed, or keeping it Draft until workflow sponsorship is available? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 927333765c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/codex/auth-context.ts # src/providers/openai-sidecar.ts # src/server/responses/compact.ts # src/server/responses/core.ts # src/server/search.ts # tests/responses-compaction-routing.test.ts
|
Current revision checkpoint: Completed review follow-up:
Validation on official stable Bun
Hosted status:
@Wibias The PR remains Ready on this current revision. Please re-review when the workflow approval and hosted evidence are available. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
171-183: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
gh run listdoes not accept jq's--arg, so this substitution likely yields an emptyci_urland permanently blocks every release.Lines 175-182 invoke:
gh run list ... --jq --arg branch "${GITHUB_REF#refs/heads/}" 'map(select(...)) | .[0].url // ""'
--argbelongs to thejqbinary. The GitHub CLI exposes--jq <expression>as a single-value flag and does not forward jq variable bindings. Two failure modes follow, and both are bad:
ghrejects--argas an unknown flag. The command substitution at line 171 returns an empty string, line 185 sees-z "$ci_url", and the step exits 1 with the "No successful Cross-platform CI run found" error at line 192.ghaccepts the flag order and--jqconsumes the literal string--argas its filter expression.branch, the branch name, and the real filter then arrive as stray positional arguments, and the jq program is invalid.In both cases
ci_urlis empty on every run. The gate fails closed, so there is no security exposure — but the release workflow can never reach the publish step. The referenced$branchvariable at line 182 would also be undefined inside the filter.Bind the branch inside the filter with
envinstead, which needs no variable binding.🔧 Proposed fix using an environment lookup inside the jq filter
ci_url="$( - # shellcheck disable=SC2016 - # `$branch` is a jq variable bound by --arg, not a shell variable, so - # the filter must stay single-quoted. - gh run list \ + # shellcheck disable=SC2016 + # `env.CI_BRANCH` is resolved by jq from the process environment, not + # by the shell, so the filter must stay single-quoted. + CI_BRANCH="${GITHUB_REF#refs/heads/}" gh run list \ --workflow ci.yml \ --commit "$GITHUB_SHA" \ --status success \ --limit 10 \ --json conclusion,headSha,url,workflowName,event,headBranch \ - --jq --arg branch "${GITHUB_REF#refs/heads/}" \ - 'map(select(.event == "push" and .headBranch == $branch)) | .[0].url // ""' + --jq 'map(select(.event == "push" and .headBranch == env.CI_BRANCH)) | .[0].url // ""' )"Run the following script to confirm the flag surface of the installed
ghrelease:#!/bin/bash # Description: Check whether `gh run list` accepts jq's --arg flag. set -euo pipefail gh --version echo "=== documented flags for 'gh run list' ===" gh run list --help 2>&1 || true echo "=== does --arg parse at all? (expect an unknown-flag error) ===" gh run list --limit 1 --json url --jq --arg branch main 'map(.url)' 2>&1 || echo "exit=$?" echo "=== control: plain --jq with an env lookup ===" CI_BRANCH=main gh run list --limit 1 --json url,headBranch \ --jq 'map(select(.headBranch == env.CI_BRANCH)) | .[0].url // ""' 2>&1 || echo "exit=$?"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 171 - 183, Update the ci_url command substitution’s gh run list invocation to remove the unsupported --arg branch arguments and expose the branch through an environment variable. Change the --jq filter to compare headBranch with that environment value via jq’s env lookup, preserving the existing push-event filtering and empty-string fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 485-497: Extract the duplicated Linux keyring isolation and unlock
logic into one committed helper script, preserving its temporary directories,
cleanup trap, permissions, isolated environment, dbus-run-session, and fail-fast
bash settings. Update .github/workflows/ci.yml lines 485-497 to invoke the
helper, and update .github/workflows/release.yml lines 108-125 to invoke the
same helper instead of duplicating the block; alternatively, remove the
redundant release step while retaining the CI gate.
In @.github/workflows/release.yml:
- Around line 95-96: The audit command is duplicated and omits the GUI
dependency tree. Update package.json’s audit:high script to audit both the root
and gui trees, replace the inline command in .github/workflows/release.yml lines
95-96 with bun run audit:high, and update scripts/release.ts lines 301-302 to
invoke the same script via runLoud; these three locations must share the single
definition.
In `@docs-site/src/content/docs/ja/reference/cli/providers-accounts.md`:
- Around line 167-186: Synchronize the operational guidance from the English
providers-accounts reference into the listed locale pages:
docs-site/src/content/docs/ja/reference/cli/providers-accounts.md (lines
167-186), docs-site/src/content/docs/ko/reference/cli/providers-accounts.md
(lines 167-186),
docs-site/src/content/docs/ru/reference/cli/providers-accounts.md (lines
218-237), and
docs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.md (lines
201-219). Add translated guidance covering canonical vault placement, the safe
cleanup boundary for crash residue, and preview-layout migration, ensuring no
locale contradicts the English source.
In `@scripts/keyring-smoke.ts`:
- Around line 39-41: Zero the keyring-returned readback buffer after comparing
or copying it, alongside the existing cleanup of stored and secret in the
smoke-test flow. Update the handling around entry.getSecret and the cleanup
block so every plaintext buffer, including readback, is wiped without changing
the validation behavior.
- Around line 66-71: Update the Object.defineProperty call in the cleanupFailed
handling of the keyring smoke operation to set enumerable: true for the attached
cleanupError property, preserving the existing configurable and value behavior
so compound failures include cleanup details in diagnostics.
In `@src/cli/account-main.ts`:
- Around line 116-137: Update maintainStageLease around the apiJson heartbeat
request to enforce a deadline before leaseExpiresAt with a safety margin, using
an abort mechanism that cancels a request that never settles. When the deadline
expires, terminate child and return the lost result so the existing
stage-cancellation flow runs; add coverage with a never-settling fetchImpl
verifying child termination and cancellation.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 171-183: Update the ci_url command substitution’s gh run list
invocation to remove the unsupported --arg branch arguments and expose the
branch through an environment variable. Change the --jq filter to compare
headBranch with that environment value via jq’s env lookup, preserving the
existing push-event filtering and empty-string fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d8828a1-5008-405a-817f-8925b31a7c49
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.yml.github/workflows/release.ymldevlog/_plan/260801_native_main_profiles/000_design.mddevlog/_plan/260801_native_main_profiles/001_validation.mddevlog/_plan/260801_native_main_profiles/002_implementation.mddocs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mdpackage.jsonscripts/keyring-smoke.tsscripts/release.tssrc/cli/account-api.tssrc/cli/account-main.ts
# Conflicts: # src/codex/auth-api.ts # src/providers/quota.ts # tests/provider-quota.test.ts
[GD] Verdict: changes-requestedPR: Semantic propagation
UsefulnessReal, spec-backed capability for issue Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
GateNot draft. Only remaining blocker: approve and confirm required CI green on Bottom lineAll substantive findings from this review are fixed on the current head with regression tests, and the base conflict is resolved. This is merge-ready modulo the operational gate: a green required-CI run on |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/providers/quota.ts (2)
1110-1121: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winDeduplicate asynchronous pool-key computation
cacheKeyWithAggregationStateawaitslistCodexAuthAccountsSnapshotbeforeinflightadmission. Concurrent callers can receive different snapshots becausefetchMainAccountInfoAttemptreturns an empty snapshot while another caller holds the native-main claim. They then derive differentcodex-poolkeys and run separate provider probes. Compute the aggregation key inside a shared synchronous-key flight, or add single-flight handling for the snapshot. Movinginflight.setalone does not fix this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/quota.ts` around lines 1110 - 1121, Deduplicate the asynchronous snapshot/key computation before provider probing: update the flow around cacheKeyWithAggregationState and inflight so concurrent callers share one in-progress aggregation-key operation, rather than deriving keys from inconsistent listCodexAuthAccountsSnapshot results. Ensure callers converge on the same codex-pool key and existing invalidation/forceRefresh semantics remain intact; moving inflight admission without sharing snapshot computation is insufficient.
100-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cacheKeyWithAggregationStateis correctly privacy-safe; one small durability note on the sort.The comment on line 124 and the body of
quotaSignatureValueagree: only percentages, reset timestamps, and boolean state enter the digest. Account ids and email addresses stay out of the cache key.hasCodexPoolProvideralso correctly excludesdirectmode, which matches the direct-mode early return infetchChatGptForwardQuotaat line 268.One note on line 120:
localeCompareis locale-sensitive. The digest is only ever compared against another digest computed in the same process, so ordering instability across locales cannot change behavior today. If this signature is ever persisted or compared across processes, switch to a plain codepoint comparison so the digest becomes reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/quota.ts` around lines 100 - 150, Update quotaSignatureValue’s customWindows sorting to use a deterministic codepoint-based comparison instead of localeCompare, ensuring persisted or cross-process digests have stable ordering. Preserve the existing label-based sort and all other signature fields unchanged.tests/provider-quota.test.ts (1)
1013-1021: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLine 1019 is a reference-identity check that passes on the buggy path too; only line 1020 carries signal.
Trace what
expect(next).not.toBe(racedResponse)actually proves. Suppose the racing probe wrongly committed its pre-change result to the cache. The later non-forced call on line 1018 would then take thecacheFreshfast path and returncache!.response. That object was constructed atsrc/providers/quota.tsline 1171 as{ ...response, reports }, which is a different object identity fromracedResponse. Sonot.toBepasses in both the correct and the incorrect case. The assertion cannot fail for the reason the test name states.Line 1020 is the assertion that does the work: it requires
currentAccountto reflectactiveCodexAccountId = "added", which a wrongly committed pre-change cache entry would not produce.Two improvements:
- Replace line 1019 with an assertion that the racing probe did not publish. Call
fetchProviderQuotaReports(config)twice after the race and assert that the second call returns the same object as the first. That provesnextperformed the commit, and therefore that the racing probe did not.- Use the
providerQuotaBeforePublishForTestshook that this PR added. The current gating relies on the mockedfetchresolving before the publish step, so the ordering between theactiveCodexAccountIdflip and the commit-key recheck atsrc/providers/quota.tsline 1136 is implicit. The hook makes that ordering explicit and removes the dependency on fetch-call sequencing.🔧 Proposed fix: assert the commit owner instead of object identity
const racing = fetchProviderQuotaReports(config, true); await started; config.activeCodexAccountId = "added"; release(); const racedResponse = await racing; const next = await fetchProviderQuotaReports(config); - expect(next).not.toBe(racedResponse); expect(next.reports[0]?.aggregation?.currentAccount).toMatchObject({ plan: "prolite", quota: { weeklyPercent: 77 } }); + // The racing probe must not have published; `next` is the probe that committed. + expect(next).not.toBe(racedResponse); + expect(await fetchProviderQuotaReports(config)).toBe(next);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/provider-quota.test.ts` around lines 1013 - 1021, Strengthen the race test around fetchProviderQuotaReports: use the providerQuotaBeforePublishForTests hook to explicitly pause the racing request before its publish/commit-key recheck, then change the post-race assertions so the first non-forced call commits the result and a second non-forced call returns that same response object. Preserve the currentAccount assertion to verify the updated account is reflected, and remove the ineffective direct comparison with racedResponse.src/codex/auth-api.ts (1)
1009-1014: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the main-account quota values and
updatedAtfrom one snapshot.MainAccountInfo.quotaexplicitly omitsupdatedAt(src/codex/main-account-cache.ts:6-10), so do not accessmainInfo.quota.updatedAt.parseUsageQuotaaccepts reset-credit-only responses, whilesetAccountQuotaFromParsedcan advanceupdatedAtand preserve older usage values (src/codex/quota.ts:134-179, 360-367). Return and cache one stored quota snapshot, or invalidate the cached quota when the store write does not commit. This preventsaggregateCodexPoolCapacityfrom applying freshness checks to a different quota vintage (src/providers/codex-capacity.ts:185-203).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/auth-api.ts` around lines 1009 - 1014, Update the main-account quota flow around quotaForPlan to use one stored quota snapshot for both returned values and updatedAt, rather than combining mainInfo.quota with a separate getAccountQuota result. Reuse the snapshot produced after setAccountQuotaFromParsed, including preserved usage and reset-credit-only updates, and invalidate the cached quota when the store write does not commit so aggregateCodexPoolCapacity never sees mixed quota vintages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/quota.ts`:
- Around line 1107-1109: Update src/providers/quota.ts lines 1107-1109 in the
prefetchedCodexSnapshot initialization to prefetch whenever
hasCodexPoolProvider(config) is true, passing forceRefresh as the probe
argument. Add a sibling regression test in tests/provider-quota.test.ts lines
524-545 that calls fetchProviderQuotaReports(config) without forceRefresh and
verifies the calls map records one probe per account.
In `@tests/keyring-smoke.test.ts`:
- Around line 58-70: Extend the “zeros the buffer returned by the keyring” test
to capture the Buffer created by createRandomBytes, then assert that this
generated secret is also zeroed after runKeyringSmoke completes. Preserve the
existing readback assertion and ensure the captured secret is distinct from the
keyring-returned buffer.
- Around line 90-91: Update the assertion in the error-handling test near the
existing Object.keys(error) check to inspect the nested cleanupError directly:
assert cleanupError.message contains “could not delete the temporary entry”
instead of relying on Bun.inspect(error), while preserving the enumerability
assertion unchanged.
---
Outside diff comments:
In `@src/codex/auth-api.ts`:
- Around line 1009-1014: Update the main-account quota flow around quotaForPlan
to use one stored quota snapshot for both returned values and updatedAt, rather
than combining mainInfo.quota with a separate getAccountQuota result. Reuse the
snapshot produced after setAccountQuotaFromParsed, including preserved usage and
reset-credit-only updates, and invalidate the cached quota when the store write
does not commit so aggregateCodexPoolCapacity never sees mixed quota vintages.
In `@src/providers/quota.ts`:
- Around line 1110-1121: Deduplicate the asynchronous snapshot/key computation
before provider probing: update the flow around cacheKeyWithAggregationState and
inflight so concurrent callers share one in-progress aggregation-key operation,
rather than deriving keys from inconsistent listCodexAuthAccountsSnapshot
results. Ensure callers converge on the same codex-pool key and existing
invalidation/forceRefresh semantics remain intact; moving inflight admission
without sharing snapshot computation is insufficient.
- Around line 100-150: Update quotaSignatureValue’s customWindows sorting to use
a deterministic codepoint-based comparison instead of localeCompare, ensuring
persisted or cross-process digests have stable ordering. Preserve the existing
label-based sort and all other signature fields unchanged.
In `@tests/provider-quota.test.ts`:
- Around line 1013-1021: Strengthen the race test around
fetchProviderQuotaReports: use the providerQuotaBeforePublishForTests hook to
explicitly pause the racing request before its publish/commit-key recheck, then
change the post-race assertions so the first non-forced call commits the result
and a second non-forced call returns that same response object. Preserve the
currentAccount assertion to verify the updated account is reflected, and remove
the ineffective direct comparison with racedResponse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11d65cd4-f560-49c2-8646-db01a1db3dca
⛔ Files ignored due to path filters (1)
gui/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci.yml.github/workflows/release.ymldocs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mdgui/package.jsonpackage.jsonscripts/keyring-smoke.tsscripts/release.tssrc/cli/account-api.tssrc/cli/account-main.tssrc/codex/auth-api.tssrc/providers/quota.tstests/ci-workflows.test.tstests/cli-native-profile.test.tstests/keyring-smoke.test.tstests/provider-quota.test.tstests/release-helper.test.ts
|
Current revision checkpoint:
Validation on official Bun
GitHub Actions on this exact head: React Doctor, Service lifecycle, and Cross-platform CI all completed successfully. The PR remains Ready and mergeable; requested human reviews are still pending. |
[GD] Verdict: changes-requestedPR: Semantic propagation
UsefulnessReal, spec-backed capability for issue Bugs / correctnessAll findings from the prior verdict round are fixed with regression coverage: same-process POSIX lock release (shared-fd registry + dedicated test), Unix interpreter-launch probe gap (contract tests), deadline restart handoff race (listener-stop + readiness budget + child-exit rejection), and the staging heartbeat stall (deadline-raced heartbeat, 30s safety margin, deterministic clock tests). New this round — CodeRabbit threads, verified against the code at
Local tip: core suites verified on prior heads and the fix commits; the full suite is covered by CI (green). SecurityCrypto and credential handling remain sound (AES-256-GCM, AAD binding, OS keyring custody, fail-closed staging cleanup, management auth/CSRF/origin). Prior keyring-smoke minors (readback zeroization, enumerable Spec / standardsSpec: issue Reviews
Base / CI
GateNot draft. Remaining before merge-ready: address or decline the Bottom lineAll substantive review findings are fixed, the base is clean, and required CI is green on |
[GD] Addressed feedbackfeedbacks:
commit: 2736ae1 |
|
Thanks @luvs01 — merging this. Why it helps: this adds encrypted native-main profiles with OS-keyring key custody, transactional switching with crash recovery, and admission fencing for issue #656; every finding from the review loop (POSIX lock ownership, process probe, restart handoff, staging heartbeat, keyring CI) is fixed with regression coverage on Ship it. |
3 participants
Summary
Add an opt-in, CLI/backend-only native-main profile workflow under
ocx account main:doctor,list,register,add,switch, andrecover. It changes the physical Codex login in the effectiveCODEX_HOME, is independent from Pool routing, and preserves task/history files.Each native Codex
auth.jsonis handled as an exact opaque envelope. Inactive profiles and the recovery journal use AES-256-GCM with AAD binding and a random master key held by the native OS credential store through@napi-rs/keyring. The feature supports Codex credential-store modefileonly and fails closed for unavailable keyring, unsupportedkeyring/auto/ephemeralmodes, invalid layouts, and home/path mismatches. There is no plaintext key-file or shell fallback.Safety and recovery
__main__work, requires native Codex to be stopped, atomically publishes the exact target bytes, verifies read-back, updates encrypted vault/runtime state, and restores the exact source on failure.__main__traffic while an unresolved journal exists; health, management, Direct, and ordinary Pool traffic remain available.RECOVERY_REQUIRED; list, doctor, cancel, recover, and switch recovery remain available.codex loginruns in an isolated restricted home. Plaintext staging credentials are removed on success, failure, cancellation, and expiry.__main__runtime-derived state is reconciled; tasks and history remain untouched.Management API boundary
The native-profile management routes are behind the existing local management authentication, CSRF, and origin gates. They carry labels, profile IDs, and staging IDs only; they do not accept or return auth envelopes, access tokens, refresh tokens, raw account IDs, or decrypted vault payloads. Real-server route-admission tests cover missing/wrong admin auth, hostile origins, GUI session/CSRF rejection, and valid admin or GUI admission.
Maintainer follow-up addressed
bun audit --audit-level=highrelease/CI gates and supported-OS keyring create/read/delete smoke jobs.devrebase, corrected CLI documentation, pending-recovery mutation guard, and refreshed-target rollback preservation.Validation
bun audit --audit-level=high: passed.git diff --check upstream/dev...HEAD: passed.A broad local root-suite run completed 6,918 passing and 6 skipped tests, with five fixed 5-second timeout overruns in unchanged account-store/auth-context/routing tests. The run took about 1,555 seconds versus the suite's roughly 210-second normal-duration warning, and isolated failures exposed the existing Windows ACL/timing environment rather than native-profile assertions. Hosted CI on this exact head is the authoritative cross-platform result.
The docs-site frozen install currently reports that the existing upstream
bun.lockwould change before a docs build can start. This PR does not rewrite that unrelated lockfile state.Scope
No dashboard UI, Pool-to-native credential conversion, native process termination, non-file credential-store support, or plaintext fallback is included.
Fixes #656
Related context: #821, #823
Summary by CodeRabbit
ocx account main, including listing, registration, switching, diagnostics, and recovery.